PR to add accuracy verification to auto_scheduler #21
Conversation
| 3rdparty/dmlc-core/src/io/line_split.cc | ||
| 3rdparty/dmlc-core/src/io/local_filesys.cc | ||
| 3rdparty/dmlc-core/src/io/recordio_split.cc | ||
| ) |
There was a problem hiding this comment.
could you please point which function requires compilation of this set of files?
There was a problem hiding this comment.
This is required for serialization of ref tensors in measure_records.cc https://github.com/Deelvin/tvm/pull/21/files#diff-64f00aa6e5a03e1cee25959caacb7e33bf4fb4d829ab0f1825ccfdfc459add51
|
|
||
|
|
||
| virtual Array<tvm::runtime::NDArray> GetOutput(const Array<MeasureInput>& inputs, | ||
| const Array<BuildResult>& build_results, int verbose) = 0; |
There was a problem hiding this comment.
Documentation string is required.
GetXXX sounds like a getter, but suppose it's one more version of method Run with reported outputs.
| LayoutRewriteOption layout_rewrite_option; | ||
| /*! \brief Names of some user defined input data used in program measuring. */ | ||
| Array<String> task_input_names; | ||
| /*! \brief keeping custom seed to reproduce randomly generated input values */ |
There was a problem hiding this comment.
Comments should start from capital letter.
|
|
||
| void SetReferenceTensors(Array<tvm::runtime::NDArray> arr); | ||
|
|
||
| void SetTarget(Target target, Target target_host); |
There was a problem hiding this comment.
Why do we need these setters? All this fields are public.
| /*! \brief Names of some user defined input data used in program measuring. */ | ||
| Array<String> task_input_names; | ||
| /*! \brief keeping custom seed to reproduce randomly generated input values */ | ||
| int custom_seed; |
There was a problem hiding this comment.
I still wondering about random generator behaviour on different platforms. I guess different std library may produce different random arrays with same seed. I don't think that using seed is acceptable in that case.
|
|
||
| #faked ref to extract task | ||
| #TODO: without it, initial serialization crashes... how to solve it more elegantly? | ||
| ref = [tvm.nd.empty((1, 1))] |
There was a problem hiding this comment.
Very suspicious comment. Looks like constructor of SearchTask object with default arguments produces invalid object. I guess it should be clarified and improved.
Looks like None value in field of type Array<NDArray> leads to wrong searialization.
| original_target_host = task.target_host | ||
|
|
||
| ref_target = tvm.target.Target("llvm", host="llvm") | ||
| _ffi_api.SetTarget(task, ref_target, None) |
There was a problem hiding this comment.
rewriting targets field of original task is not elegant solution. Is it possible to make a copy of search task object with ref_taget?
|
|
||
| results = local_runner.get_ouput(measure_inputs, build_results) | ||
|
|
||
| _ffi_api.SetReferenceTensors(task, results) |
There was a problem hiding this comment.
Using of function _ffi_api.SetReferenceTensors is a workaround. I'm personally sure that ffi infrastructure allow to set and get values of native object without special setting methods.
| from . import _ffi_api | ||
|
|
||
|
|
||
| import tvm |
There was a problem hiding this comment.
where do you use tvm module in this file? Do you really need it? I guess from tvm.target import Target will be better.
| TVM_REGISTER_GLOBAL("tvm.contrib.random.random_fill").set_body([](TVMArgs args, TVMRetValue* ret) { | ||
| RandomThreadLocalEntry* entry = RandomThreadLocalEntry::ThreadLocal(); | ||
| DLTensor* out = args[0]; | ||
| int seed = args[1]; |
There was a problem hiding this comment.
There are a lot of usage random_fill function with only one argument. By this you changed semantic and now all of them will crash. Please make it optional.
| i.Save(fs); | ||
| } | ||
|
|
||
| delete fs; |
There was a problem hiding this comment.
Using of external tmp file for serialisation is not nice. It may confuse some one (including me). Moreover it cannot be easily enhanced to RPC case.
First option is to use direct serialisation into json.
example:
std::string bytes;
runtime::NDArray arr = data.ref_output_tensors[0];
dmlc::MemoryStringStream stream(&bytes);
ci.Save(&stream);
writer->WriteArrayItem(bytes);
It has a critical issue, because "bytes" array may contain special chars like ", \t, \n and other. Alternative way to keep it as hex.
Second option is to change the place where we keep "ref_output_tensors". I'm not sure that "SearchTask" object is the best choice.
| error_msg = make_traceback_info() | ||
|
|
||
| shutil.rmtree(os.path.dirname(build_res.filename)) | ||
| toc = time.time() |
|
|
||
| shutil.rmtree(os.path.dirname(build_res.filename)) | ||
| toc = time.time() | ||
| time.sleep(cooldown_interval) |
There was a problem hiding this comment.
there is no loop for which we would do this cooldown
| number, | ||
| repeat, | ||
| min_repeat_ms, | ||
| cooldown_interval, |
There was a problem hiding this comment.
cooldown_interval is not seen as necessary.
it might make sense to use(cooldown_interval_ms, limit_zero_time_iteration, repeats_to_cooldown) for func.time_evaluator
| loc_args = [] | ||
|
|
||
| # pylint: disable=consider-using-enumerate | ||
| for idx in range(len(args)): |
There was a problem hiding this comment.
for idx, arg in enumerate(args): will simplify the code a little bit
| dev.sync() | ||
| costs = time_f(*loc_args).results | ||
|
|
||
| idx = len(loc_args) - 1 |
3 participants
@apeskov , @elvin-n - guys, would you please review and provide your feedback